Fiet 370 - #23
Conversation
WalkthroughAdds a new Action.FetchFees enum and client/server support to request fee metadata for a specified symbol on a CEX. Client now issues FetchFees for Changes
Sequence Diagram(s)mermaid Client->>Proto: send ExecuteAction(Action.FetchFees, { cex: "mexc", symbol }) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✏️ Tip: You can disable this entire section by setting Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server.ts (1)
216-241: Fix typo and update “account ID” error text to “fees”.The new FetchFees path still logs/returns account‑ID messaging and has a “requied” typo.
✏️ Proposed fix
- message: `ValidationError: Symbol requied`, + message: `ValidationError: Symbol required`, ... - log.error(`Error fetching account ID ${cex}:`, error); + log.error(`Error fetching fees for ${cex}:`, error); ... - message: `Error fetching account ID from ${cex}`, + message: `Error fetching fees from ${cex}`,
🤖 Fix all issues with AI agents
In `@src/client.dev.ts`:
- Around line 72-76: The call to client.executeAction with Action.FetchFees is
missing the required symbol field causing an INVALID_ARGUMENT error; update the
request object passed to client.executeAction (the call site using
Action.FetchFees) to include a symbol property (e.g., symbol: "<MARKET_SYMBOL>"
or the appropriate variable) alongside cex and action so the server receives the
required symbol parameter.
In `@src/server.ts`:
- Around line 230-234: The response currently returns broker.fees directly which
can be undefined; update the callback payload so that the result JSON always
includes an explicit generalFee value set to the broker's fee when present or
the string "unknown" when absent, and emit a warning when fees are missing (use
the existing logger used in this module, e.g., processLogger.warn or
console.warn) before returning; keep the rest of the returned fields (proof:
verityProof, market) unchanged and ensure result is JSON.stringified as before.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/client.dev.tssrc/proto/node.protosrc/server.ts
🔇 Additional comments (1)
src/proto/node.proto (1)
59-59: Enum extension looks good.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| client.executeAction( | ||
| { | ||
| cex: "mexc", | ||
| action: Action.FetchAccountId, | ||
| action: Action.FetchFees, | ||
| }, |
There was a problem hiding this comment.
Add symbol to FetchFees request to avoid guaranteed INVALID_ARGUMENT.
Server-side FetchFees now requires symbol, so this call will fail without it.
🛠️ Proposed fix
client.executeAction(
{
cex: "mexc",
+ symbol: "ETHUSDT",
action: Action.FetchFees,
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| client.executeAction( | |
| { | |
| cex: "mexc", | |
| action: Action.FetchAccountId, | |
| action: Action.FetchFees, | |
| }, | |
| client.executeAction( | |
| { | |
| cex: "mexc", | |
| symbol: "ETHUSDT", | |
| action: Action.FetchFees, | |
| }, |
🤖 Prompt for AI Agents
In `@src/client.dev.ts` around lines 72 - 76, The call to client.executeAction
with Action.FetchFees is missing the required symbol field causing an
INVALID_ARGUMENT error; update the request object passed to client.executeAction
(the call site using Action.FetchFees) to include a symbol property (e.g.,
symbol: "<MARKET_SYMBOL>" or the appropriate variable) alongside cex and action
so the server receives the required symbol parameter.
There was a problem hiding this comment.
Why do you replace FetchAccountId? After applying theses changes fiet-prover is unable to get account Id from neither sandbox nor real binance endpoint.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the Action enum definition to see if FetchAccountId still exists
echo "=== Action enum definition ==="
rg -n "enum Action" -A 20 src/proto/
echo -e "\n=== All references to FetchAccountId ==="
rg -n "FetchAccountId"
echo -e "\n=== All references to FetchFees ==="
rg -n "FetchFees"
echo -e "\n=== Check if fiet-prover is mentioned in the codebase ==="
rg -n "fiet-prover"
Length of output: 959
🏁 Script executed:
#!/bin/bash
# Check the server.ts switch statement for Action handling
echo "=== Server.ts Action.FetchFees case ==="
rg -n "case Action.FetchFees" -B 2 -A 25 src/server.ts
echo -e "\n=== Check all case statements in the switch ==="
rg -n "case Action\." src/server.ts
echo -e "\n=== Check default case in switch ==="
rg -n "default:" -A 5 src/server.ts
Length of output: 1990
@victorshevtsov You're absolutely right to raise this concern. This is a breaking change that removes existing functionality.
The Problem
Looking at the code:
Action.FetchAccountIdstill exists in the enum (line 58 ofsrc/proto/node.proto)Action.FetchFeeswas added as a new action (line 59)- However, in
src/server.ts, the case handler forFetchAccountIdwas replaced withFetchFees
This means any requests with Action.FetchAccountId (from fiet-prover or other consumers) will now hit the default case and fail with INVALID_ARGUMENT: "Invalid Action".
The Fix
Both actions should coexist. The case Action.FetchAccountId: handler needs to be restored in src/server.ts alongside the new case Action.FetchFees: handler. Each should serve its distinct purpose:
FetchAccountId→ returns account identifierFetchFees→ returns fee metadata and market info
Could you check what the original FetchAccountId implementation returned? It needs to be added back to the switch statement to restore backward compatibility with fiet-prover and other consumers.
✏️ Learnings added
Learnt from: victorshevtsov
Repo: usherlabs/cex-broker PR: 23
File: src/client.dev.ts:72-76
Timestamp: 2026-01-29T17:09:48.143Z
Learning: In the cex-broker codebase, Action.FetchAccountId must remain implemented in src/server.ts to support external consumers like fiet-prover that depend on fetching account IDs from CEX endpoints; removing or replacing this handler breaks backward compatibility.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| // Return normalized response | ||
| return callback(null, { | ||
| proof: verityProof, | ||
| result: JSON.stringify({ accountId }), | ||
| result: JSON.stringify({ generalFee: broker.fees, market }), | ||
| }); |
There was a problem hiding this comment.
Return explicit “unknown” fee metadata when unavailable (and warn).
Acceptance criteria call for explicit “unknown” with a warning; broker.fees may be absent, but the response currently serializes it as-is.
🛠️ Proposed fix
- return callback(null, {
- proof: verityProof,
- result: JSON.stringify({ generalFee: broker.fees, market }),
- });
+ const generalFee = broker.fees ?? null;
+ const feeStatus = broker.fees ? "ok" : "unknown";
+ if (!broker.fees) {
+ log.warn(`Fee metadata unavailable for ${cex}`, { symbol });
+ }
+ return callback(null, {
+ proof: verityProof,
+ result: JSON.stringify({ generalFee, feeStatus, market }),
+ });🤖 Prompt for AI Agents
In `@src/server.ts` around lines 230 - 234, The response currently returns
broker.fees directly which can be undefined; update the callback payload so that
the result JSON always includes an explicit generalFee value set to the broker's
fee when present or the string "unknown" when absent, and emit a warning when
fees are missing (use the existing logger used in this module, e.g.,
processLogger.warn or console.warn) before returning; keep the rest of the
returned fields (proof: verityProof, market) unchanged and ensure result is
JSON.stringified as before.
There was a problem hiding this comment.
Here is the same question. Also, please consider coderabbit's review
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all comments by victorshevtsov in this PR to understand "the same question"
gh pr view 23 --json comments,reviews --jq '.comments[] | select(.author.login == "victorshevtsov") | {body, createdAt, url}'
# Also get review comments
gh api repos/usherlabs/cex-broker/pulls/23/comments --jq '.[] | select(.user.login == "victorshevtsov") | {body, path, line, created_at}'
Length of output: 390
@victorshevtsov, I found your earlier question on src/client.dev.ts - you're absolutely right to be concerned. Replacing Action.FetchAccountId with Action.FetchFees breaks backward compatibility with fiet-prover and any other external consumers that depend on fetching account IDs from CEX endpoints.
The Issue
As documented in the learnings, Action.FetchAccountId must remain implemented to support external consumers. The current PR removes it entirely, which is a breaking change.
Recommended Solution
Keep both actions - don't replace FetchAccountId, add FetchFees as a new action alongside it:
case Action.FetchAccountId: {
try {
let accountId = await broker.fetchAccountId();
// Return normalized response
return callback(null, {
proof: verityProof,
result: JSON.stringify({ accountId }),
});
} catch (error) {
log.error(`Error fetching account ID ${cex}:`, error);
callback(
{
code: grpc.status.INTERNAL,
message: `Error fetching account ID from ${cex}`,
},
null,
);
}
break;
}
case Action.FetchFees: {
if (!symbol) {
return callback(
{
code: grpc.status.INVALID_ARGUMENT,
message: `ValidationError: Symbol required`,
},
null,
);
}
try {
await broker.loadMarkets();
const market = await broker.market(symbol);
// Address CodeRabbit's concern: explicit handling for missing fees
const generalFee = broker.fees ?? null;
const feeStatus = broker.fees ? "available" : "unknown";
if (!broker.fees) {
log.warn(`Fee metadata unavailable for ${cex}`, { symbol });
}
return callback(null, {
proof: verityProof,
result: JSON.stringify({ generalFee, feeStatus, market }),
});
} catch (error) {
log.error(`Error fetching fees for ${symbol} from ${cex}:`, error);
callback(
{
code: grpc.status.INTERNAL,
message: `Error fetching fees from ${cex}`,
},
null,
);
}
break;
}This approach:
- ✅ Preserves backward compatibility for
fiet-prover - ✅ Adds the new fee metadata functionality
- ✅ Addresses CodeRabbit's concern with explicit "unknown" status and warning
- ✅ Aligns with PR objectives to provide fee metadata while maintaining existing functionality
Would you like me to create an issue to track restoring the FetchAccountId handler?
🧬 Code Graph Analysis Results
[src/helpers/logger.ts] (line 9-9)
log
[src/types.ts] (line range 24-32)
export type PolicyConfig = {
withdraw: {
rule: WithdrawRule;
};
deposit: Record<string, null>;
order: {
rule: OrderRule;
};
};
[src/types.ts] (line range 170-170)
export type brokers = Required<BrokerMap>;
[src/helpers/index.ts] (line range 15-27)
export function authenticateRequest<T, E>(
call: ServerUnaryCall<T, E>,
whitelistIps: string[],
): boolean {
const clientIp = call.getPeer().split(":")[0];
if (whitelistIps.includes("*")) {
return true;
} else if (!clientIp || !whitelistIps.includes(clientIp)) {
log.warn(`Blocked access from unauthorized IP: ${clientIp || "unknown"}`);
return false;
}
return true;
}
[src/helpers/index.ts] (line range 214-238)
export function selectBroker(
brokers:
| {
primary: Exchange;
secondaryBrokers: Exchange[];
}
| undefined,
metadata: Metadata,
): Exchange | null {
if (!brokers) {
return null;
} else {
const use_secondary_key = metadata.get("use-secondary-key");
if (!use_secondary_key || use_secondary_key.length === 0) {
return brokers.primary;
} else if (use_secondary_key.length > 0) {
const keyIndex = Number.isInteger(
+(use_secondary_key[use_secondary_key.length - 1] ?? "0"),
);
return brokers.secondaryBrokers[+keyIndex] ?? null;
} else {
return null;
}
}
}
[src/helpers/index.ts] (line range 87-121)
export function createBroker(
cex: string,
credsOrMetadata: { apiKey: string; apiSecret: string } | Metadata,
): Exchange | null {
let apiKey: string | undefined;
let apiSecret: string | undefined;
// Duck-typing check for gRPC Metadata (has get/remove functions)
if (
credsOrMetadata &&
typeof (credsOrMetadata as unknown as { get: unknown }).get ===
"function" &&
typeof (credsOrMetadata as unknown as { remove: unknown }).remove ===
"function"
) {
const metadata = credsOrMetadata as Metadata;
apiKey = metadata.get("api-key")?.[0]?.toString();
apiSecret = metadata.get("api-secret")?.[0]?.toString();
metadata.remove("api-key");
metadata.remove("api-secret");
} else {
const creds = credsOrMetadata as { apiKey: string; apiSecret: string };
apiKey = creds.apiKey;
apiSecret = creds.apiSecret;
}
const ExchangeClass = (ccxt.pro as Record<string, typeof Exchange>)[cex];
if (!ExchangeClass || !apiKey || !apiSecret) {
return null;
}
const exchange = new ExchangeClass({ apiKey, secret: apiSecret });
applyCommonExchangeConfig(exchange);
return exchange;
}
[src/server.ts] (line range 62-75)
export function buildHttpClientOverrideFromMetadata(
metadata: Metadata,
verityProverUrl: string,
onProofCallback: (proof: string, notaryPubKey?: string) => void,
): HttpClientOverride {
const redact = metadata.get("verity-t-redacted")?.[0]?.toString() || "";
const rawTimeout = metadata.get("verity-proof-timeout")?.[0]?.toString();
const proofTimeout = rawTimeout ? parseInt(rawTimeout, 10) : 5 * 60 * 1000; // default 5 minutes
const factory = createVerityHttpClientOverride(
verityProverUrl,
onProofCallback,
);
return factory(redact, proofTimeout);
}
[src/server.ts] (line range 77-85)
export const verityHttpClientOverridePredicate: HttpOverridePredicate = ({
method,
methodCalled,
}) => {
return (
["get", "post"].includes(method.toLowerCase()) &&
CCXT_METHODS_WITH_VERITY.includes(methodCalled)
);
};
[src/server.ts] (line range 311-361)
export function validateWithdraw(
policy: PolicyConfig,
network: string,
recipientAddress: string,
amount: number,
ticker: string,
): { valid: boolean; error?: string } {
const withdrawRule = policy.withdraw.rule;
// Check if network is allowed
if (!withdrawRule.networks.includes(network)) {
return {
valid: false,
error: `Network ${network} is not allowed. Allowed networks: ${withdrawRule.networks.join(", ")}`,
};
}
// Check if address is whitelisted
if (!withdrawRule.whitelist.includes(recipientAddress.toLowerCase())) {
return {
valid: false,
error: `Address ${recipientAddress} is not whitelisted for withdrawals`,
};
}
// Check amount limits
const amountRule = withdrawRule.amounts.find((a) => a.ticker === ticker);
if (!amountRule) {
return {
valid: false,
error: `Ticker ${ticker} is not allowed. Supported tickers: ${withdrawRule.amounts.map((a) => a.ticker).join(", ")}`,
};
}
if (amount < amountRule.min) {
return {
valid: false,
error: `Amount ${amount} is below minimum ${amountRule.min}`,
};
}
if (amount > amountRule.max) {
return {
valid: false,
error: `Amount ${amount} exceeds maximum ${amountRule.max}`,
};
}
return { valid: true };
}
[src/server.ts] (line range 366-419)
export function validateOrder(
policy: PolicyConfig,
fromToken: string,
toToken: string,
amount: number,
broker: string,
): { valid: boolean; error?: string } {
const orderRule = policy.order.rule;
// Check if market is allowed
const marketKeys = [
`${broker.toUpperCase()}:${toToken}/${fromToken}`,
`${broker.toUpperCase()}:${fromToken}/${toToken}`,
];
if (
!(
orderRule.markets.includes(marketKeys[0] ?? "") ||
orderRule.markets.includes(marketKeys[1] ?? "")
)
) {
return {
valid: false,
error: `Market ${marketKeys} is not allowed. Allowed markets: ${orderRule.markets.join(", ")}`,
};
}
// Check conversion limits
const limit = orderRule.limits.find(
(l) => l.from === fromToken && l.to === toToken,
);
if (!limit) {
return {
valid: false,
error: `Conversion from ${fromToken} to ${toToken} is not allowed`,
};
}
if (amount < limit.min) {
return {
valid: false,
error: `Amount ${amount} is below minimum ${limit.min} for ${fromToken} to ${toToken} conversion`,
};
}
if (amount > limit.max) {
return {
valid: false,
error: `Amount ${amount} exceeds maximum ${limit.max} for ${fromToken} to ${toToken} conversion`,
};
}
return { valid: true };
}
🧠 Learnings used
Learnt from: victorshevtsov
Repo: usherlabs/cex-broker PR: 23
File: src/client.dev.ts:72-76
Timestamp: 2026-01-29T17:09:48.143Z
Learning: In the cex-broker codebase, Action.FetchAccountId must remain implemented in src/server.ts to support external consumers like fiet-prover that depend on fetching account IDs from CEX endpoints; removing or replacing this handler breaks backward compatibility.
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.